Switch HasMethodType/HasPropertyType from ObjectTypeTrait to MaybeObjectTypeTrait#5605
Switch HasMethodType/HasPropertyType from ObjectTypeTrait to MaybeObjectTypeTrait#5605phpstan-bot wants to merge 13 commits intophpstan:2.1.xfrom
Conversation
… object type contains template types - When the specific `method_exists` handling in `ImpossibleCheckTypeHelper` cannot determine the result (no concrete class names, no GenericClassStringType), it falls through to the general type specifier logic - The general logic uses `HasMethodType::isSuperTypeOf()` which calls `hasMethod()` on the argument type; for `object&T` where T is a TemplateMixedType, this returns Yes (inherited from MixedType), causing a false positive "will always evaluate to true" - Add early `return null` when `$objectType->hasTemplateOrLateResolvableType()` is true, preventing the general logic from running on types where method existence is uncertain - Verified analogous cases: `property_exists` is not affected (its type specifying extension returns empty SpecifiedTypes for non-native properties); `is_callable` is not affected; `class-string<T>` template bounds are correctly handled by the existing GenericClassStringType block
VincentLanglet
left a comment
There was a problem hiding this comment.
If the issue seems related to TemplateMixedType, why it doesn't currently occur for MixedType ?
Demonstrates that the false positive only occurs with template types (object&T), not with plain mixed or object. For mixed, ObjectWithoutClassType::isSuperTypeOf(MixedType) returns Maybe (mixed is not necessarily an object), preventing the false positive. For plain object, ObjectWithoutClassType::hasMethod() returns Maybe (via ObjectTypeTrait), also preventing it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the answer to the reviewer's question: Why doesn't The false positive requires two conditions to align in the general type specifier logic:
For plain For plain For I added test cases for the |
VincentLanglet
left a comment
There was a problem hiding this comment.
Should/could the fix be done in the general specifier ?
…o return Maybe A template type T represents a specific-but-unknown type. MixedType::hasMethod() returns Yes (mixed encompasses all types including those with any method), but for a template T we don't know whether the specific type has a given method. This fixes the root cause instead of working around it in ImpossibleCheckTypeHelper. The previous workaround checked hasTemplateOrLateResolvableType() in the method_exists-specific handling to bail out early. With this fix, the general type specifier logic works correctly because HasMethodType::isSuperTypeOf() now gets Maybe (not Yes) from TemplateMixedType, so it no longer concludes that method_exists() on object&T "will always evaluate to true." Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The fix now addresses the reviewer's suggestion: instead of working around the issue in Why this is the right fix: All 11,995 tests pass and |
VincentLanglet
left a comment
There was a problem hiding this comment.
List other possible fixes cause this one is not ok.
MixedType:: and TemplateMixedType methods shouldn't have different result.
…lper Revert the hasMethod() override on TemplateMixedType so that it inherits MixedType::hasMethod() (returning Yes) as before — the two should not diverge. Instead, prevent the method_exists specific handling in ImpossibleCheckTypeHelper from falling through to the general type specifier logic when the object type contains template or late-resolvable types. The general logic incorrectly concludes "always true" because MixedType::hasMethod() returns Yes, which dominates the IntersectionType result via lazyMaxMin. Alternatives considered: - HasMethodType::isSuperTypeOf(): could check for template types, but would incorrectly return Maybe for bounded templates like T of SomeClass where the method genuinely exists on the bound. - IntersectionType::hasMethod(): changing from lazyMaxMin to lazyMin would be a large behavioral change affecting all intersections. - General result processing: capping results at Maybe for template types is equivalent to this fix but less targeted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. The fix now addresses the reviewer's concern: What changed: Reverted the Why the general logic gives the wrong answer for Alternatives considered:
|
VincentLanglet
left a comment
There was a problem hiding this comment.
I feel like the fix should be inside HasMethodType::isSuperTypeOf() instead.
For instance HasMethodType::isSuperTypeOf(Mixed) should be Maybe only (this would be consistent with MixedType::isSubtypeOf(HasMethod) which is maybe)
…ompoundTypes HasMethodType::isSuperTypeOf(MixedType) was returning Yes (via MixedType::hasMethod() which always returns Yes), inconsistent with MixedType::isSubTypeOf(HasMethodType) which returns Maybe. This caused false positives for method_exists() on template types like object&T, where TemplateMixedType (inheriting MixedType::hasMethod()) dominated IntersectionType::hasMethod() via lazyMaxMin. The fix delegates to $type->isSubTypeOf($this) for CompoundType instances (the standard pattern used by other accessory types like AccessoryNonEmptyStringType). When isSubTypeOf returns No, we fall back to hasMethod() — this prevents AccessoryTypes in intersections (e.g. AccessoryLiteralStringType in class-string&literal-string) from being overly strict, since their isSubTypeOf checks orthogonal properties that can incorrectly poison the lazyMaxMin result. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I processed this review but have nothing to report. |
|
why don't we have the same bug for |
|
Indeed, I'll ask the bot |
VincentLanglet
left a comment
There was a problem hiding this comment.
why don't we have the same bug for property_exists?
https://phpstan.org/r/01359ac6-3f8e-4ea2-b6f3-82b1e2de8b3f
… CompoundTypes Same pattern as the HasMethodType fix: delegate to $type->isSubTypeOf($this) for CompoundTypes so that MixedType correctly returns Maybe instead of Yes. property_exists() doesn't currently trigger the same false positive as method_exists() because PropertyExistsTypeSpecifyingExtension bails out early for non-native properties (which MixedType provides). But the underlying type inconsistency is the same: HasPropertyType::isSuperTypeOf(MixedType) was returning Yes when it should return Maybe. Added property_exists test cases to bug-8217 regression test to cover the template, mixed, and object scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the answer to the reviewer's question: Why The false positive requires the general type specifier logic to conclude "always true." For For But the underlying type inconsistency was the same: I also investigated |
VincentLanglet
left a comment
There was a problem hiding this comment.
Just use
if ($type instanceof CompoundType) {
return $type->isSubTypeOf($this);
}
to fix this issue and explains why it impacts class-string&literal-string.
…delegation Use the standard pattern `return $type->isSubTypeOf($this)` instead of falling back to hasMethod()/hasProperty() when isSubTypeOf() returns No. This is consistent with how other AccessoryType classes handle CompoundTypes. The simplified delegation changes behavior for `class-string&literal-string`: since IntersectionType::isSubTypeOf(HasMethodType) uses lazyMaxMin over individual members, and neither ClassStringType nor AccessoryLiteralStringType has methods, the result becomes No instead of the previous Maybe. To prevent this from causing false "always false" reports for method_exists() on class-string types (where the actual class is unknown), add explicit return null in ImpossibleCheckTypeHelper for both generic class-string types (non-final TypeWithClassName) and plain class-string types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the summary: Changes made:
Why this impacts With the simplified delegation, The dedicated |
VincentLanglet
left a comment
There was a problem hiding this comment.
I don't understand your explanation
With the simplified delegation, HasMethodType::isSuperTypeOf(class-string&literal-string) returns No instead of the previous Maybe. This happens because IntersectionType::isSubTypeOf(HasMethodType) uses lazyMaxMin over the individual intersection members (ClassStringType, AccessoryLiteralStringType), and neither has methods — both return No from HasMethodType::isSuperTypeOf().
Because both ClassStringType::hasMethod and AccessoryLiteralStringType::hasMethod are supposed to return MAYBE
…() returns No AccessoryLiteralStringType implements CompoundType, so HasMethodType::isSuperTypeOf(AccessoryLiteralStringType) was delegating to AccessoryLiteralStringType::isSubTypeOf(HasMethodType), which returns No (because HasMethodType::isLiteralString() is No). This made HasMethodType::isSuperTypeOf(class-string&literal-string) return No instead of Maybe, since lazyMaxMin propagated the No from the accessory type even though ClassStringType::hasMethod() correctly returned Maybe. The fix: when the CompoundType delegation returns No, fall back to the hasMethod()/hasProperty() check which gives the correct result. This removes the need for the class-string guard in ImpossibleCheckTypeHelper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the explanation for the reviewer: Why the The reviewer is correct that both The actual issue: The fix: When the CompoundType delegation returns |
VincentLanglet
left a comment
There was a problem hiding this comment.
Looking at code like
$objectType = $scope->getType($args[0]->value);
if ($objectType->isString()->yes()) {
if ($objectType->isClassString()->yes()) {
return $this->typeSpecifier->create(
$args[0]->value,
new IntersectionType([
$objectType,
new HasMethodType($methodNameType->getValue()),
]),
$context,
$scope,
);
}
return new SpecifiedTypes([], []);
}
I feel like the issue is that HasMethod::isString (and similar method) return No because the ObjectTrait is used while it should return Maybe WDYT ?
…to return Maybe HasMethodType and HasPropertyType use ObjectTypeTrait which returns No for isString(), isClassString(), isLiteralString(), etc. But these accessory types are agnostic about whether the underlying type is an object or a class-string — method_exists() and property_exists() work on both. Returning No breaks CompoundType delegation: when AccessoryLiteralStringType::isSubTypeOf(HasMethodType) checks HasMethodType::isLiteralString(), it gets No and incorrectly concludes the types are incompatible. Override the 9 string/scalar methods to return Maybe, keeping ObjectTypeTrait for object-related behavior (isObject, canCallMethods, etc.). This fixes the CompoundType delegation and removes the need for the ImpossibleCheckTypeHelper workaround. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the summary of what was done: The reviewer was right: What changed:
|
| if ($type instanceof CompoundType) { | ||
| $result = $type->isSubTypeOf($this); | ||
| if (!$result->no()) { | ||
| return $result; | ||
| } | ||
| } |
There was a problem hiding this comment.
As a proof this works, the implementation should now be
if ($type instanceof CompoundType) {
return $type->isSubTypeOf($this);
}
There was a problem hiding this comment.
Done. Simplified the isSuperTypeOf() CompoundType delegation in both HasMethodType and HasPropertyType to unconditionally return $type->isSubTypeOf($this) — the fallback-on-No guard is no longer needed since the string/scalar Maybe overrides now ensure correct results through the delegation path. All 11,997 tests pass and make phpstan reports no errors.
VincentLanglet
left a comment
There was a problem hiding this comment.
HasMethodType and HasPropertyType should rely on MaybeObjectTypeTrait rather than ObjectTypeTrait (and maybe override some property).
Also, a MaybeStringTypeTrait might be useful to deduplicate some methods
…delegation Now that string/scalar type queries return Maybe on HasMethodType and HasPropertyType, the fallback-on-No guard is no longer needed. The CompoundType delegation can unconditionally return the isSubTypeOf() result. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deduplicate the 9 string/scalar type query methods (isString, isNumericString, isNonEmptyString, isNonFalsyString, isLiteralString, isLowercaseString, isUppercaseString, isClassString, isScalar) that return Maybe into a reusable MaybeStringTypeTrait. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ectTypeTrait These accessory types can be intersected with both objects and class-strings, so they should not claim to definitely be objects. Replace ObjectTypeTrait with MaybeObjectTypeTrait plus explicit sub-traits (MaybeCallableTypeTrait, MaybeIterableTypeTrait, MaybeOffsetAccessibleTypeTrait, NonArrayTypeTrait, TruthyBooleanTypeTrait) and MaybeStringTypeTrait for the string/scalar queries. Methods that were provided by ObjectTypeTrait but not by the new traits are now defined explicitly on each class. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All 11,997 tests pass and Here's a summary of the changes: Commit 1: Extract
Commit 2: Switch
|
| return TrinaryLogic::createMaybe(); | ||
| } | ||
|
|
||
| public function getCallableParametersAcceptors(ClassMemberAccessAnswerer $scope): array |
There was a problem hiding this comment.
getCallableParametersAcceptors is already in MaybeCallableTypeTrait
There was a problem hiding this comment.
Pushed. Here's a summary of the changes addressing each review comment:
-
Removed redundant
getCallableParametersAcceptors()fromHasMethodType— already provided byMaybeCallableTypeTrait(and removed unusedTrivialParametersAcceptorimports from both files) -
Moved
getConstantStrings()intoMaybeStringTypeTrait— removed the duplicategetConstantStrings()returning[]fromHasMethodType,HasPropertyType,HasOffsetType, andHasOffsetValueType -
getClassStringObjectType()returns$thisinstead ofErrorType— this preserves method/property info when resolving class-string intersections. ReturningObjectWithoutClassType(as initially suggested) loses the method info, which breaks callable detection aftermethod_exists()checks (e.g.,new $factory()aftermethod_exists($factory, '__invoke')) -
getObjectTypeOrClassStringObjectType()keeps returning$this— same rationale.NewHandleruses this method to resolvenew $variable()types, and returningObjectWithoutClassTypeloses theHasMethodTypeinfo needed for callable checks
| return []; | ||
| } | ||
|
|
||
| public function getConstantStrings(): array |
There was a problem hiding this comment.
I think this can be in MaybeStringTrait ? And I dunno if some other method should be added in the MaybeStringTypeTrait.
|
|
||
| public function getClassStringObjectType(): Type | ||
| { | ||
| return new ErrorType(); |
There was a problem hiding this comment.
This should be ObjectWithoutClassType
|
|
||
| public function getObjectTypeOrClassStringObjectType(): Type | ||
| { | ||
| return $this; |
There was a problem hiding this comment.
This should maybe return ObjectWithoutClassType
…t overrides, fix getClassStringObjectType - Move getConstantStrings() into MaybeStringTypeTrait, removing duplicates from HasMethodType, HasPropertyType, HasOffsetType, HasOffsetValueType - Remove redundant getCallableParametersAcceptors() from HasMethodType (already provided by MaybeCallableTypeTrait) - Remove unused TrivialParametersAcceptor imports - Change getClassStringObjectType() to return $this on HasMethodType and HasPropertyType so that class-string intersections preserve method/property info (needed for e.g. new $classString() after method_exists check) - Keep getObjectTypeOrClassStringObjectType() returning $this (same reason) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
Suggest a better title and description for the PR now
|
The token doesn't have write access to the upstream repo. Since there are no code changes to make (this is just a title/description update request), I'll report the suggested title and description for the user to apply. The bot token doesn't have permission to edit the PR on New title:
New description: Summary
Root cause
Test plan
Closes phpstan/phpstan#8217 The token lacks write access to |
method_exists() check fall through to general logic when object type contains template types
Summary
HasMethodTypeandHasPropertyTypefromObjectTypeTraittoMaybeObjectTypeTrait, so string/scalar/object type queries returnMaybeinstead of definitive answers — these accessory types can appear in intersections with both objects and class-stringsisSuperTypeOf()in both types to delegate toCompoundType::isSubTypeOf(), makingHasMethodType::isSuperTypeOf(MixedType)correctly returnMaybeinstead ofYesMaybeStringTypeTraitto deduplicate string-relatedMaybemethods acrossHasMethodType,HasPropertyType,HasOffsetType, andHasOffsetValueTypeRoot cause
HasMethodTypeandHasPropertyTypeusedObjectTypeTrait, which madeisString(),isScalar(), etc. returnNo. This caused incorrect results when these accessory types appeared in intersections with string-like types (e.g.class-string & literal-string & hasMethod(foo)). Additionally,isSuperTypeOf()did not delegate toCompoundType::isSubTypeOf(), soHasMethodType::isSuperTypeOf(MixedType)returnedYes(becauseMixedType::hasMethod()returnsYes), leading to false positive "will always evaluate to true" reports formethod_exists()onobject & Ttemplate types.Test plan
method_exists()onobject & T(bug-8217.php)method_exists/property_existswithmixedandobjecttypesHasMethodType::isSuperTypeOf(MixedType)=MaybeHasPropertyType::isSuperTypeOf(MixedType)=Maybemake tests— all 11,997 tests passmake phpstan— no errorsCloses phpstan/phpstan#8217